Skip to content

refactor(storage): make onResponse async and improve retry error handling - #24

Open
thiyaguk09 wants to merge 11 commits into
mainfrom
fix/resumable-upload-error-serialization
Open

refactor(storage): make onResponse async and improve retry error handling#24
thiyaguk09 wants to merge 11 commits into
mainfrom
fix/resumable-upload-error-serialization

Conversation

@thiyaguk09

Copy link
Copy Markdown
Owner

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the resumable upload logic to make onResponse asynchronous and introduces a helper function formatRetryError to standardize retry error formatting. The reviewer suggested refactoring formatRetryError to eliminate code duplication between the Error instance and generic object handling blocks.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread handwritten/storage/src/resumable-upload.ts Outdated
Comment thread handwritten/storage/src/resumable-upload.ts Outdated
@thiyaguk09 thiyaguk09 changed the title refactor(storage): make onResponse async and improve error formatting for retry failures refactor(storage): make onResponse async and improve retry error handling Jun 25, 2026
@thiyaguk09
thiyaguk09 force-pushed the fix/resumable-upload-error-serialization branch 2 times, most recently from fb994fb to 794370b Compare July 10, 2026 12:30
@thiyaguk09
thiyaguk09 force-pushed the fix/resumable-upload-error-serialization branch 5 times, most recently from a162ec6 to a797680 Compare July 27, 2026 06:48
@thiyaguk09
thiyaguk09 force-pushed the fix/resumable-upload-error-serialization branch 5 times, most recently from c587bcd to 434e34f Compare July 31, 2026 13:49
quirogas and others added 10 commits July 31, 2026 08:19
…kflow (googleapis#9033)

Update `discovery.yaml` to fix failures on future workflows. 

- Update the PR title and commit message to use `chore(bigquery)`, so
release-please component can identify the change correctly.
- Add a cleanup step that removes temporary `pnpm-lock.yaml` files and
reverts unintended formatting changes to handwritten source files.
- Sets the `git_dif` key to `.` to the correct path. 

> NOTE: since handwritten packages do not track pnpm lockfiles running
pnpm install creates one.
Ran the following commands on librarian version 0.31.1:

```
go install github.com/googleapis/librarian/cmd/librarian@0.31..1
librarian install nodejs
librarian generate --all 
```
## Description

Upgrade all generated packages to Node 22 along with their dependencies.

For all `package.json` files inside `packages/**` folder we set "node":
">=22" for `engines`. We also make sure that for all of these
`package.json` files the dependencies that match the core libraries are
all on the latest version so that all core library dependencies
correspond to node v22.

Note that a branch update is likely required to get the zizmor checks to
pass.

Also note that upgrading the auth library dependencies causes compiler
errors so we opt out of upgrading the auth library.

## Impact

Encourages customers to adopt new Node version which avoids issues due
to lack of Node 18 support.

## Next Steps

Upgrade the auth dependencies that we chose not to upgrade this time due
to the compiler error. Tracked
[here](https://b.corp.google.com/issues/541318013).
Now that we've migrated to librarian for new API libraries, this can go
away.
…pis#9030)

While trying to get the [handwritten split project](googleapis#8934) finished, I ran into a lot of resource leaks in the bigtable system tests. These ended up being extensive enough changes that I moved them to their own PR (this one).
…googleapis#9023)

## Summary
This PR implements the core `Client`, `Pool`, and `Query` execution
interfaces for `@google-cloud/spanner-driver`, providing compatibility
with `node-postgres` (`pg`) driver layer for Google Cloud Spanner

---

## Key Changes

### 1. `Client` Connection & Execution Queue (`src/lib/client.ts`)
- **`Client` Class**: Implements `node-postgres` compatible `Client`
handle managing connection state (`isConnected`), transaction status
tracking (`txStatus: 'I' | 'T' | 'E'`), and DSN resolution.
- **Sequential Task Queue (`queryQueue`)**: Enforces sequential query
execution order per client connection handle.
- **Transaction Status Lifecycle**: Updates `txStatus = 'I'` only after
statement execution completes (`COMMIT`, `ROLLBACK`, `ABORT`).
- **`release()` Method**: Added `client.release()` method delegating to
connection teardown for `node-postgres` compatibility.
- **Unhandled Error Prevention**: Added `query.listenerCount('error') >
0` check prior to emitting `'error'` events, preventing Node process
crashes when queries are consumed via Promises (`await client.query()`).

### 2. `Query` Class & Thenable / EventEmitter Integration
(`src/lib/query.ts`)
- **Dual Invocation Model**: Extends `EventEmitter` for row streaming
(`.on('row', cb)`, `.on('end', cb)`) while implementing the Thenable
interface (`then`, `catch`, `finally`) for `async`/`await` support.
- **Overload Support**: Supports query strings, `QueryConfig` objects,
`Query` instances, positional value arrays (`$1`, `$2`), and Node
callbacks (`(err, res) => void`).
- **Constructor Robustness**:
- Added `text !== null` guard when initializing from objects (`typeof
text === 'object' && text !== null`).
- Added support for overriding positional `values` and `callback` when
instantiating from an existing `Query` instance.

### 3. `Pool` Scaffolding & Callback Single-Invocation Rule
(`src/lib/pool.ts`)
- **`Pool` Class**: Implements `connect()`, `query()`, and `end()`.
- **Client Binding**: Binds `client.release = client.end.bind(client)`
during client acquisition with `TODO(PR 4 - Connection Pooling)` markers
for pool recycling in PR 4.
- **Single Invocation Guarantee**: Isolated `_doConnect()` connection
acquisition error handling from query execution, ensuring connection
failures call callbacks exactly once without hanging or double
callbacks.
- **3rd-Argument Callback Overload Resolution**: Resolved 3rd-argument
callback parameters when executing `pool.query(query, values, callback)`
or `client.query(query, values, callback)`.
- Unify formatting of Errors and objects under a single block inside
formatRetryError to reduce code duplication.
- Ensure standard errors, GaxiosErrors, and custom errors with
empty/missing properties are correctly formatted.
@thiyaguk09
thiyaguk09 force-pushed the fix/resumable-upload-error-serialization branch from 434e34f to 5a7ab72 Compare August 3, 2026 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants